Skip to content

modules: hal_rpi_pico: Introducing -std=gnu11 option #84974

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Merged
merged 1 commit into from
Feb 11, 2025
Merged

modules: hal_rpi_pico: Introducing -std=gnu11 option #84974

merged 1 commit into from
Feb 11, 2025

Conversation

soburi
Copy link
Member

@soburi soburi commented Jan 31, 2025 •
edited
Loading

The original Pico-SDK is compiled with -std=gnu11 option.
Aligning with it.

@zephyrbot zephyrbot added the size: XS A PR changing only a single line of code label Jan 31, 2025
@zephyrbot
Copy link
Collaborator

zephyrbot commented Jan 31, 2025 •
edited
Loading

The following west manifest projects have changed revision in this Pull Request:

Name Old Revision New Revision Diff

✅ All manifest checks OK

Note: This message is automatically posted and updated by the Manifest GitHub Action.

@zephyrbot zephyrbot added manifest manifest-hal_rpi_pico DNM This PR should not be merged (Do Not Merge) labels Jan 31, 2025
@soburi soburi added bug The issue is a bug, or the PR is fixing a bug platform: Raspberry Pi Pico Raspberry Pi Pico (RPi Pico) labels Jan 31, 2025
@carlescufi
Copy link
Member

carlescufi commented Jan 31, 2025 •
edited
Loading

@ajf58 could you review please? (I see you've reviewed the module PR already). Also, would you consider becoming a collaborator in this area? You have already made extensive contributions and your reviews would then "count" (green tick) in this the main repo, which would help the project enormously. It would be a matter of opening a PR adding your GitHub username here:

zephyr/MAINTAINERS.yml

Line 3561 in 4abffff

collaborators:

This was referenced Feb 1, 2025
Merged
Merged
Open
@soburi soburi requested a review from ThreeEights February 4, 2025 01:22
@soburi soburi self-assigned this Feb 4, 2025
@fabiobaltieri fabiobaltieri added DNM (manifest) This PR should not be merged (controlled by action-manifest) and removed DNM This PR should not be merged (Do Not Merge) labels Feb 4, 2025
@soburi soburi added this to the v4.1.0 milestone Feb 4, 2025
@soburi soburi force-pushed the fix_rpi_pico2_ci_failure branch from d63373d to 3824be3 Compare February 5, 2025 03:41
@zephyrbot zephyrbot requested a review from yonsch February 5, 2025 03:42
@zephyrbot zephyrbot removed manifest manifest-hal_rpi_pico DNM (manifest) This PR should not be merged (controlled by action-manifest) labels Feb 5, 2025
@soburi
modules: hal_rpi_pico: Introducing -std=gnu11 option
4d7d20e 
The original Pico-SDK is compiled with `-std=gnu11` option.
Aligning with it.

Signed-off-by: TOKITA Hiroshi <tokita.hiroshi@gmail.com>
@soburi soburi force-pushed the fix_rpi_pico2_ci_failure branch from 3824be3 to 4d7d20e Compare February 5, 2025 03:43
@soburi soburi added the DNM This PR should not be merged (Do Not Merge) label Feb 5, 2025
@soburi soburi changed the title manifest: rpi_pico: Pull fix for CI failure modules: hal_rpi_pico: Introducing -std=gnu11 option Feb 5, 2025
@soburi soburi mentioned this pull request Feb 5, 2025
Closed
@soburi soburi changed the title modules: hal_rpi_pico: Introducing -std=gnu11 option modules: hal_rpi_pico: Introducing -std=gnu11 option Feb 5, 2025
@soburi
Copy link
Member Author

soburi commented Feb 5, 2025

@carlescufi

Could you please review this?
Also, as mentioned in the discussion at zephyrproject-rtos/hal_rpi_pico#9, do you think it would be a good idea to get @stephanosio's opinion as well?

@soburi soburi requested a review from carlescufi February 5, 2025 09:15
ThreeEights
ThreeEights approved these changes Feb 7, 2025
View reviewed changes
Copy link
Collaborator

@ThreeEights ThreeEights left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Makes sense. Approved.

This was referenced Feb 9, 2025
Closed
Merged
@ajf58
Copy link
Collaborator

ajf58 commented Feb 10, 2025

LGTM (but I'm biased).

@stephanosio has been a bit quiet on a few issues (including SDK-ng). I don't think anything related to MISRA/safety critical kernel is pertinent here, and I hope we can get this done ahead of the 4.1 release.

@fabiobaltieri
Copy link
Member

This is for typeof right? Wonder if that would end up causing problems with other toolchains down the road. That said diverging the hal code base is a guaranteed nightmare so guess let's try.

@soburi
Copy link
Member Author

soburi commented Feb 11, 2025

This is for typeof right? Wonder if that would end up causing problems with other toolchains down the road. That said diverging the hal code base is a guaranteed nightmare so guess let's try.

Yes. It's "that issue."
It's a manageable amount of work at the moment, so I was actually quite hesitant, but since it doesn't seem to affect function-safety and the problem is limited to the hal part, I'll go with this for now.

@soburi soburi removed the DNM This PR should not be merged (Do Not Merge) label Feb 11, 2025
@soburi
Copy link
Member Author

soburi commented Feb 11, 2025

@carlescufi @stephanosio

If you have time, could you authorize it?

@kartben
Copy link
Collaborator

kartben commented Feb 11, 2025

@carlescufi @stephanosio

If you have time, could you authorize it?

I am not sure there is anything else to do that for you to approve as the assignee, @soburi :)

@soburi
Copy link
Member Author

soburi commented Feb 11, 2025

I am not sure there is anything else to do that for you to approve as the assignee, @soburi :)

As @fabiobaltieri said in his comment, I think this issue has a potentially wide-reaching impact, and I would like to have someone with knowledge of this issue and someone who has a good understanding of the overall situation review it.

@fabiobaltieri
Copy link
Member

fabiobaltieri commented Feb 11, 2025 •
edited
Loading

I say it's a valid stop gap, if it's not optimal it can be reworked later, easy enough to roll it back and redo it differently.

@fabiobaltieri fabiobaltieri merged commit 4cb8761 into zephyrproject-rtos:main Feb 11, 2025
28 of 29 checks passed
@soburi soburi deleted the fix_rpi_pico2_ci_failure branch February 11, 2025 14:53
@soburi
Copy link
Member Author

soburi commented Feb 11, 2025

I say it's a valid stop gap, if it's not optimal it can be reworked later, easy enough to roll it back and redo it differently.

Understood. If there are any problems, I'll try to bring it up sooner.

@RobinKastberg RobinKastberg mentioned this pull request May 12, 2025
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@kartben kartben kartben approved these changes

@fabiobaltieri fabiobaltieri fabiobaltieri approved these changes

@ThreeEights ThreeEights ThreeEights approved these changes

@yonsch yonsch Awaiting requested review from yonsch

@carlescufi carlescufi Awaiting requested review from carlescufi

Assignees

@soburi soburi

Labels
bug The issue is a bug, or the PR is fixing a bug platform: Raspberry Pi Pico Raspberry Pi Pico (RPi Pico) size: XS A PR changing only a single line of code
Projects
None yet
Milestone
v4.1.0
Development

Successfully merging this pull request may close these issues.
7 participants
@soburi @zephyrbot @carlescufi @ajf58 @fabiobaltieri @kartben @ThreeEights